server: hash setup tokens at rest#280
Closed
erikolsson wants to merge 1 commit into
Closed
Conversation
Setup tokens were stored plaintext and looked up by raw equality, so a read-only DB leak inside the 10-minute expiry window would expose every pending pairing token. Switch to the same SHA-256-at-rest pattern that apiKeys.keyHash, refresh sessions, and MCP OAuth tokens already use: hash on issuance, hash incoming token on /v1/auth/exchange, compare hashes. Raw token is returned to the caller exactly once and never re-read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThis pull request implements token hashing for setup tokens across two endpoints. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/auth/github.ts (1)
266-278:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake setup-token redemption atomic before release.
Lines 267-278 still perform a separate read (
redeemed = false) and then update. Two concurrent exchanges can both pass the read and both mint API keys. Use a single conditional redeem operation (e.g., atomic update with redeem guard + expiry guard and row return) so only one request can succeed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/auth/github.ts` around lines 266 - 278, The current flow hashes the token, selects a row from setupTokens with redeemed = false and then separately updates redeemed = true, which allows a race where two requests can both read before either update; instead perform a single atomic conditional redeem: hashToken(body.token) then run one db.update on setupTokens that sets redeemed = true with a WHERE that matches token = tokenHash AND redeemed = false AND expiresAt > new Date() and RETURN the updated row (or check affected row count) so you can detect success/failure atomically; replace the separate db.select + db.update (references: hashToken, setupTokens, db.update, db.select, st, redeemed, expiresAt) with this single conditional update and handle the case where no row is returned/affected as the invalid/expired token path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/server/src/auth/github.ts`:
- Around line 266-278: The current flow hashes the token, selects a row from
setupTokens with redeemed = false and then separately updates redeemed = true,
which allows a race where two requests can both read before either update;
instead perform a single atomic conditional redeem: hashToken(body.token) then
run one db.update on setupTokens that sets redeemed = true with a WHERE that
matches token = tokenHash AND redeemed = false AND expiresAt > new Date() and
RETURN the updated row (or check affected row count) so you can detect
success/failure atomically; replace the separate db.select + db.update
(references: hashToken, setupTokens, db.update, db.select, st, redeemed,
expiresAt) with this single conditional update and handle the case where no row
is returned/affected as the invalid/expired token path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24177cba-1fc0-4819-9fbd-d237c66e5c70
📒 Files selected for processing (2)
apps/server/src/auth/github.tsapps/server/src/user/routes.ts
5 tasks
giuseppecrj
added a commit
that referenced
this pull request
May 5, 2026
## Summary - Combines the security fixes from #280, #281, and #282 on top of `g/consolidate-server-boundaries` so those separate PRs can be closed after review. - Hashes setup tokens at rest while still returning the raw token once for pairing. - Claims setup tokens atomically during `/v1/auth/exchange` so concurrent redemption cannot mint multiple API keys. - Adds a unique index on `api_keys(device_id)` as the schema-level one-active-key-per-device guarantee. - Updates tests for the new invariant and adds an assertion that raw setup tokens are not stored in the database. ## Validation - `bun run gen:db-schema:check` - `bun run typecheck` - `bun test test/auth-resolvers.test.ts` - `bun run test` — 296 pass, 0 fail ## Notes This PR targets `g/consolidate-server-boundaries` intentionally. Once it lands into the consolidation branch, the older security PRs #280, #281, and #282 should be redundant. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes security-sensitive auth flows and adds a data-cleanup migration with a new uniqueness constraint on `api_keys`, which could affect existing tokens/devices if assumptions are wrong. > > **Overview** > **Hardens the setup-token pairing flow** by storing only a SHA-256 hash of setup tokens and updating `/v1/auth/exchange` to *atomically* claim (redeem) tokens while also checking expiry. > > **Enforces one active API key per device** by adding a unique index on `api_keys.device_id` (with a migration that deletes older duplicate keys per device) and updating tests/docs to match the new hashing and uniqueness invariants. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6cede0e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed race condition in setup token redemption that could create duplicate API keys * Enforced one-API-key-per-device constraint * **Security** * Setup tokens are now securely hashed before storage <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Erik Olsson <valross2@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
closing due to #289 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setupTokens.tokenwas stored plaintext and looked up by raw equality. A read-only DB leak (backup, replica, query log) inside the 10-minute expiry would have handed an attacker every pending pairing token, redeemable for a long-lived device API key.apiKeys.keyHash, refresh sessions, and MCP OAuth tokens already use: hash on issuance, hash the incoming token on/v1/auth/exchange, compare hashes.Compatibility
setup_tokens.tokencolumn type is unchanged (text); this is a behavior-only change. No migration required.Audit context
First of three PRs from a security review of the desktop pairing flow. PR 2 will close the TOCTOU on redemption (atomic
UPDATE … RETURNING); PR 3 will add a unique index onapi_keys.device_idso a future regression can never leave two valid keys per device.Test plan
bun run typecheck(apps/server) — cleanbun run test(apps/server) — 290 pass / 0 fail🤖 Generated with Claude Code
Note
Medium Risk
Touches the device pairing/auth exchange path: newly issued setup tokens are now hashed and exchange compares hashes, which can break any in-flight tokens during deploy and could block pairing if hashing/lookup mismatches occur.
Overview
Setup tokens used for device/CLI pairing are no longer stored in plaintext:
POST /api/me/setup-tokennow saves a SHA-256 hash while returning the raw token only in the response.POST /v1/auth/exchangenow hashes the presented token and looks upsetupTokensby hash before redeeming and issuing the device API key.Reviewed by Cursor Bugbot for commit 5fc9830. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit